[SYCL] Remove `IsDeprecatedDeviceCopyable` #16615

aelovikov-intel · 2025-01-14T00:24:35Z

This was discussed in
#15342 (comment) and the consensus seemed to be that we should drop it right away in a separate PR, do it here.

Technically, it is a breaking change that could also be considered a bugfix. An example of a class failing the updated check is

struct Kernel {
    Kernel(int);
    Kernel(const Kernel&) = default;
    Kernel& operator=(const Kernel&) { return *this; } // non-trivial
};

An additional minor reason (other than not being SYCL-conformant) to drop it right away is to save a tiny bit of compile time that is currently used to support something violating the spec.

This required some fixes in the reductions implementation to make sure the kernel we submit internally are actually device copyable.

This was discussed in intel#15342 (comment) and the consensus seemd to be that we should drop it right away in a separate PR, do it here. Technically, it is a breaking change that could also be considered a bugfix. An example of a class failing the updated check is ``` struct Kernel { Kernel(int); Kernel(const Kernel&) = default; Kernel& operator=(const Kernel&) { return *this; } // non-trivial }; ``` An additional minor reason (other than not being SYCL-conformant) to drop it right away is to save a tiny bit of compile time that is currently used to support something violating the spec.

aelovikov-intel · 2025-01-15T17:11:10Z

sycl/include/sycl/detail/is_device_copyable.hpp

+template <typename... Ts>
+struct is_device_copyable<sycl::detail::tuple<Ts...>>
+    : std::bool_constant<(... && is_device_copyable<Ts>::value)> {};
+


A better way might be just making sycl::detail::tuple trivially copyable automatically whenever all the contained types are, by fixing the implementation, but I'm not sure how big such a change would be or how to approach it.

aelovikov-intel · 2025-01-16T14:31:51Z

Ping

aelovikov-intel · 2025-01-18T00:58:08Z

Looks like this broke CTS. Root cause: https://godbolt.org/z/vqan5Y9xM, so the issue is either in the CTS or in the clang FE.

--- a/tests/handler/handler_invoke_api.h
+++ b/tests/handler/handler_invoke_api.h
@@ -43,6 +43,7 @@ struct TestConstants {
         offsetNdRange(offsetRange, offsetRange, offset),
         numWorkGroups(1),
         workGroupSize(defaultRange) {}
+  ~TestConstants() = default;
 };

 struct use_offset {

in CTS would make it buildable again.

@intel/dpcpp-cfe-reviewers , can you tell if clang's behavior is correct?

elizabethandrews · 2025-01-21T21:53:41Z

Looks like this broke CTS. Root cause: https://godbolt.org/z/vqan5Y9xM, so the issue is either in the CTS or in the clang FE.
--- a/tests/handler/handler_invoke_api.h
+++ b/tests/handler/handler_invoke_api.h
@@ -43,6 +43,7 @@ struct TestConstants {
         offsetNdRange(offsetRange, offsetRange, offset),
         numWorkGroups(1),
         workGroupSize(defaultRange) {}
+  ~TestConstants() = default;
 };

 struct use_offset {
in CTS would make it buildable again.

@intel/dpcpp-cfe-reviewers , can you tell if clang's behavior is correct?

Hmm... It is unclear why the defaulted destructor changes the behavior in clang. I also noticed that removing the const member changes the behavior. There seems to be some underlying bug but it is unclear to me whether TestConstants is trivially copyable or not. I am looking at the standards now. Tom is also looking into this.

elizabethandrews · 2025-01-21T22:19:24Z

Looks like this broke CTS. Root cause: https://godbolt.org/z/vqan5Y9xM, so the issue is either in the CTS or in the clang FE.
--- a/tests/handler/handler_invoke_api.h
+++ b/tests/handler/handler_invoke_api.h
@@ -43,6 +43,7 @@ struct TestConstants {
         offsetNdRange(offsetRange, offsetRange, offset),
         numWorkGroups(1),
         workGroupSize(defaultRange) {}
+  ~TestConstants() = default;
 };

 struct use_offset {
in CTS would make it buildable again.
@intel/dpcpp-cfe-reviewers , can you tell if clang's behavior is correct?
Hmm... It is unclear why the defaulted destructor changes the behavior in clang. I also noticed that removing the const member changes the behavior. There seems to be some underlying bug but it is unclear to me whether TestConstants is trivially copyable or not. I am looking at the standards now. Tom is also looking into this.

I discussed this with Tom and we think this is a Clang bug. Can you file a JIRA for it?

tahonermann · 2025-01-21T22:33:37Z

std::is_trivially_copyable is an odd trait since it can be satisfied in so many ways. For example, a type that is trivially move-assignable but not trivially copy/move constructible or trivially copy-assignable satisfies it. I suggest avoiding it and using the more precise std::is_trivially_copy_constructible traits to specify the actual requirements needed.

jsji · 2025-01-21T23:07:09Z

I discussed this with Tom and we think this is a Clang bug. Can you file a JIRA for it?

64941, assigned to you first.

…ntel#16615) Restore (logically) previous condition while keeping the rest of cleanups in place. Deprecation warning never worked, so I'm not even trying to implement that.

…16615) (#16744) Restore (logically) previous condition while keeping the rest of cleanups in place. Deprecation warning never worked, so I'm not even trying to implement that.

aelovikov-intel requested a review from a team as a code owner January 14, 2025 00:24

aelovikov-intel requested review from cperkinsintel, AlexeySachkov and steffenlarsen January 14, 2025 00:24

aelovikov-intel had a problem deploying to WindowsCILock January 14, 2025 00:24 — with GitHub Actions Failure

aelovikov-intel had a problem deploying to WindowsCILock January 14, 2025 00:53 — with GitHub Actions Failure

aelovikov-intel marked this pull request as draft January 14, 2025 18:31

aelovikov-intel force-pushed the remove-trait branch from aab026c to bec21d1 Compare January 14, 2025 23:41

aelovikov-intel temporarily deployed to WindowsCILock January 14, 2025 23:42 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock January 15, 2025 00:10 — with GitHub Actions Inactive

aelovikov-intel marked this pull request as ready for review January 15, 2025 17:09

aelovikov-intel commented Jan 15, 2025

View reviewed changes

cperkinsintel approved these changes Jan 17, 2025

View reviewed changes

Remove debugging asserts

109d320

aelovikov-intel temporarily deployed to WindowsCILock January 17, 2025 16:27 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock January 17, 2025 17:12 — with GitHub Actions Inactive

aelovikov-intel merged commit a5e8209 into intel:sycl Jan 17, 2025
17 checks passed

aelovikov-intel deleted the remove-trait branch January 17, 2025 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Remove `IsDeprecatedDeviceCopyable` #16615

[SYCL] Remove `IsDeprecatedDeviceCopyable` #16615

aelovikov-intel commented Jan 14, 2025 •

edited

Loading

aelovikov-intel Jan 15, 2025

aelovikov-intel commented Jan 16, 2025

aelovikov-intel commented Jan 18, 2025 •

edited

Loading

elizabethandrews commented Jan 21, 2025

elizabethandrews commented Jan 21, 2025

tahonermann commented Jan 21, 2025

jsji commented Jan 21, 2025

[SYCL] Remove IsDeprecatedDeviceCopyable #16615

[SYCL] Remove IsDeprecatedDeviceCopyable #16615

Conversation

aelovikov-intel commented Jan 14, 2025 • edited Loading

aelovikov-intel Jan 15, 2025

Choose a reason for hiding this comment

aelovikov-intel commented Jan 16, 2025

aelovikov-intel commented Jan 18, 2025 • edited Loading

elizabethandrews commented Jan 21, 2025

elizabethandrews commented Jan 21, 2025

tahonermann commented Jan 21, 2025

jsji commented Jan 21, 2025

[SYCL] Remove `IsDeprecatedDeviceCopyable` #16615

[SYCL] Remove `IsDeprecatedDeviceCopyable` #16615

aelovikov-intel commented Jan 14, 2025 •

edited

Loading

aelovikov-intel commented Jan 18, 2025 •

edited

Loading